Skip to content

feat(review-fix-loop): define invocation, checkpoint, and terminal-result contracts - #107

Merged
shaug merged 22 commits into
mainfrom
claude/96-review-fix-loop-contracts
Jul 30, 2026
Merged

feat(review-fix-loop): define invocation, checkpoint, and terminal-result contracts#107
shaug merged 22 commits into
mainfrom
claude/96-review-fix-loop-contracts

Conversation

@shaug

@shaug shaug commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

Defines the public review-fix-loop invocation, checkpoint, grant, cycle-budget,
review-record, and terminal-result contracts, per design/review-fix-loop.md,
under a new skills/review-fix-loop/ skill directory:

  • Three JSON Schemas (invocation, checkpoint, terminal-result) covering
    every field named in the design's "Invocation contract," "Durable invocation
    checkpoint," and "Terminal result contract" sections, including candidate
    worktree-cleanliness enforcement, allowed_remediation_scope, checkpoint
    worktree/validation_outcomes/preserved_failed_attempts/pull_request,
    terminal-result worktree/resume_status/unresolved_or_deferred_findings,
    and a per-pass reviewer_identity on review_records.
  • A dependency-free scripts/validate.py, mirroring the repository's
    established review-code-change/review-suite validator convention, enforcing
    both the JSON Schemas and the cross-field semantics documented in
    references/CONTRACT.md — including a "Cross-document identity invariants"
    table enumerating the complete closed field set that must agree across
    invocation/checkpoint/terminal-result, enforced symmetrically by
    validate_checkpoint_against_invocation and
    validate_terminal_against_checkpoint.
  • 8 example documents (local_commit and update_pr, including
    changes_remaining and blocked terminal states), each in canonical
    serialized form.
  • 115 unit tests covering valid, invalid, boundary, cross-document, and
    round-trip/determinism cases.

This is the first child of epic #95 and the foundation the execution (#97,
#99, #100) and reviewer-orchestration (#98) leaves depend on. Per its scope,
it does not run reviewers, apply fixes, acquire locks, manage worktrees,
recover from interruption, or publish anything.

Acceptance criteria

  • Valid examples for both local_commit and update_pr pass strict validation.
  • Invalid or ambiguous inputs fail closed with actionable diagnostics.
  • Zero-cycle and review-only requests are rejected.
  • Cycle consumption and remaining budget can be reconstructed from checkpoint history.
  • Every terminal state has an explicit publication/retained-commit/operator-action contract.
  • Contract serialization is deterministic and covered by complete snapshots.

Validation

  • python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py' — 115 tests, pass.
  • just format — pass, no reformatting needed at head.
  • just lint — pass, including skills-ref validate skills/review-fix-loop.
  • just test — full repository suite (implement-epic, implement-ticket, babysit-pr,
    carve-changesets, all four review skills, review-fix-loop's own 115 tests, and
    review-suite's 310 tests) — pass, no failures.

Review history

This candidate went through 11 full review-code-change passes (each a fresh,
isolated, read-only reviewer given only raw evidence) across 10 fix cycles,
converging on a systematic completeness effort against
design/review-fix-loop.md's enumerated field lists:

  1. Converged-evidence gap in validate_terminal_result (embedded
    review_records/validation_summary not checked) — fixed.
  2. comparison_base omitted from the checkpoint/terminal-result cross-check — fixed.
  3. Empty/scope-incomplete validation_summary and initial-base cross-check gap — fixed.
  4. validate_checkpoint_against_invocation added (base/budget/repository/policy) — fixed.
  5. Missing branch/repository/policy checkpoint cross-check, allowed_remediation_scope,
    checkpoint worktree/validation_outcomes, misattributed changelog SHA — fixed.
  6. Systematic cross-document identity table + commit-provenance rules
    (created_commits/fix_commit_sha linkage) — fixed.
  7. Checkpoint schema missing preserved_failed_attempts/pull_request — fixed.
  8. Terminal-result schema missing worktree/resume_status/unresolved_or_deferred_findings,
    bound-source ahead_by/behind_by not enforced — fixed.
  9. Missing per-pass reviewer_identity, invocation worktree-cleanliness never enforced — fixed.
  10. Converged accepted a mutation_attempts on a non-final review record — fixed.

The eleventh (final authorized) pass found one more item, recorded below as a
disclosed follow-up rather than triggering a twelfth cycle.

Known follow-up

The eleventh review-code-change pass found one blocking gap, reproduced
directly against the checked-in examples, that this PR intentionally does not
fix (final authorized review cycle for this round):

validate_terminal_result never checks that a validation_summary entry
with status passed/failed carries result, or that an unavailable
entry carries reason — even though validate_checkpoint enforces exactly
this for the identically-shaped validation_outcomes, and CONTRACT.md
explicitly documents that terminal-result's collection "follows the same
status/result/reason shape as every other validation collection in this
contract family." Reproduced directly: stripping result from a converged
example's passed entries, and appending diagnostically-empty failed/
unavailable entries to changes_remaining and blocked examples, all
pass validation with zero errors. No test exercises this path either.

Proposed fix: add a loop in validate_terminal_result over
validation_summary (mirroring validate_checkpoint's existing loop over
validation_outcomes) that rejects a passed/failed entry missing result
and an unavailable entry missing reason, plus a covering test analogous
to the existing checkpoint tests.

Guardrail evidence

Diff is ~4,160 insertions across 17 files — well above the "few hundred lines"
preferred range in carve-changesets' cognitive-load guardrails. Classified as
within guardrails rather than oversized:

  • It is one cohesive deliverable — the invocation, checkpoint, and
    terminal-result contracts for a single new skill — not a mix of unrelated
    concerns; the three schemas and their two cross-document validator functions
    are inherently interdependent (the "Cross-document identity invariants"
    table only makes sense reviewed together), so splitting by schema would
    actively harm reviewability rather than help it.
  • A large share of the line count is repetitive, low-cognitive-load content:
    8 JSON example fixtures and ~1,165 lines of largely formulaic unit tests.
    validate.py itself is ~500 lines of validation logic.
  • Eleven independent fresh-reviewer passes across this session each
    successfully reasoned about the complete diff as one unit, reproduced
    specific claims against it, and gave precise line-level citations —
    empirical evidence the diff is within a single reviewer's cognitive
    capacity despite its raw size.
  • This ticket's authority explicitly withholds decomposition authority and
    instead requires reporting guardrail evidence rather than carving, which
    this section does.

Refs #96
Supports #95

shaug added 22 commits July 30, 2026 10:43
…sult contracts

## Summary
- Add the skill-local `review-fix-loop` schemas (invocation, checkpoint,
  terminal-result) plus a dependency-free `scripts/validate.py` and its
  cross-field semantics documented in `references/CONTRACT.md`.
- Model fix-cycle budgets as invocation-scoped state reconstructed from
  checkpoint `cycle_attempts` history rather than stored denormalized counts.
- Give every terminal state (`converged`, `changes_remaining`, `blocked`) an
  explicit, validator-enforced publication/retained-commit/operator-action
  contract, covering both `local_commit` and `update_pr`.
- Reject zero-cycle and unsupported review-only invocations, and ambiguous
  source-binding/publication-policy combinations, with actionable diagnostics.
- Add complete valid examples for both publication policies (including
  `changes_remaining` and `blocked` terminal results), each already in
  canonical serialized form, plus 62 unit tests covering valid, invalid,
  boundary, cross-document consistency, and round-trip/determinism cases.

## Why
- Issue #96 is the first child of epic #95 ("Build a standalone
  review-fix-loop skill") and is the foundation the execution
  (#97/#99/#100) and reviewer-orchestration (#98) leaves depend on. This
  child intentionally implements no reviewer, fix, lock, worktree, or
  publication behavior — only the contracts those leaves will consume.

Refs #96
Supports #95
Backfill the full SHA onto the prior verification-sufficiency-removal
bullet and add today's new review-fix-loop contracts entry, per this
repository's changelog convention (AGENTS.md).
…d evidence

## Summary
- `validate_terminal_result` now rejects `terminal_state: converged` unless
  its own embedded evidence actually supports it: every `validation_summary`
  entry must be `passed`, and `review_records` must contain an entry bound to
  the exact final head and comparison base whose `aggregate_verdict` is
  `clean` and `write_isolation` is `enforced`.
- Add five negative tests reproducing the exact gap review-code-change found:
  a failed or unavailable required validation entry, a non-clean or
  write-isolation-violated final-head review record, and a missing final-head
  review record all now fail closed instead of silently validating.
- Document the new check in `references/CONTRACT.md`.

## Why
- The initial review-code-change pass found this a blocking correctness gap:
  CONTRACT.md already claimed "the validator enforces the combination so a
  result cannot claim a terminal state without the evidence that state
  requires," but `validate_terminal_result` never actually inspected
  `review_records` or `validation_summary`, so a document could claim
  `converged` while its own embedded evidence showed a failing check, an open
  finding, or a compromised reviewer boundary. This mirrors the sibling
  review-suite validator's existing "clean cannot pair with failed
  validation" rule.

Refs #96
Supports #95
Backfill SHAs onto the two prior review-fix-loop entries and add today's
new fix entry, per this repository's changelog convention.
…l-result cross-check

## Summary
- `validate_terminal_against_checkpoint` now also compares the checkpoint's
  current `comparison_base.sha` against the terminal result's
  `comparison_base.final.sha`, matching CONTRACT.md's existing documented
  claim that this function confirms "head/base identities" match.
- Add a negative test proving a terminal result with a fabricated
  comparison base, otherwise internally self-consistent, is now rejected
  against the real checkpoint.

## Why
- The second review-code-change pass found this a blocking correctness gap:
  the function only ever compared head SHAs and budget, silently omitting
  the base-identity leg CONTRACT.md already documented it as covering. A
  terminal result could report a comparison base its own checkpoint never
  recorded and still pass this integrity check.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
…se history match

## Summary
- `_check_converged_requires_clean_evidence` now rejects `converged` when
  `validation_summary` is empty or missing its required `focused` or `full`
  scope, not only when a present entry has failed.
- `validate_terminal_against_checkpoint` now also compares the terminal
  result's `comparison_base.initial.sha` against the checkpoint's
  `base_revision_history[0].sha`, closing the remaining half of the
  head/base reconciliation CONTRACT.md documents.
- Add four negative tests reproducing both gaps: an empty
  `validation_summary`, a `validation_summary` missing its `full` scope, and
  a terminal result with a fabricated initial comparison base.

## Why
- The third review-code-change pass found both blocking: a `converged`
  result with an empty or scope-incomplete `validation_summary` still
  claimed "required validation passed" with zero errors, and the
  checkpoint/terminal-result cross-check compared only the final
  comparison base, never the initial one, despite CONTRACT.md already
  documenting it as reconciling "head/base identities" and "history."

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
…s-check

## Summary
- Add `validate_checkpoint_against_invocation(invocation, checkpoint)`,
  symmetric to the existing `validate_terminal_against_checkpoint`: it
  cross-checks `initial_head`, `base_revision_history[0].sha`,
  `original_cycle_budget`, `invocation_id`, `repository`, and
  `publication.policy` against the invocation the checkpoint derives from.
- Correct CONTRACT.md's prior overclaim that `validate_checkpoint` alone
  enforces `base_revision_history[0]` against "the invocation's original
  comparison-base SHA" — nothing inside a checkpoint document alone can
  prove that, since it has no other field to compare against; only the new
  cross-document function can.
- Add 8 tests: two positive (both example invocation/checkpoint pairs) and
  six negative, one per cross-checked field.
- Document the new function in SKILL.md's importable-functions list.

## Why
- Authorized by the coordinator to exceed the default three-cycle
  fix/re-review budget by exactly one additional cycle for this ticket
  only, to close the gap the fourth review-code-change pass found:
  `validate_checkpoint` never verified `base_revision_history[0]` against
  anything but itself, even though CONTRACT.md already documented that as
  an enforced rule. The terminal-result side already had the symmetric
  check (`validate_terminal_against_checkpoint`); the checkpoint side did
  not.

Refs #96
Supports #95
…elog

Backfill the SHA onto the prior review-fix-loop entry and add today's new
fix entry, per this repository's changelog convention.
…erated fields

## Summary
- `validate_terminal_against_checkpoint` now also cross-checks `repository`,
  `branch`, and `publication.policy` between a terminal result and its
  checkpoint, mirroring the equivalent checks already present in
  `validate_checkpoint_against_invocation`.
- Add `change_contract.allowed_remediation_scope` (required) to
  `invocation.schema.json`, and `worktree` and `validation_outcomes`
  (both required) to `checkpoint.schema.json`, matching the exact fields
  `design/review-fix-loop.md` enumerates for the change contract and the
  durable checkpoint. `validation_outcomes` follows the established
  status/result/reason cross-field shape used by every other validation
  collection in this contract family.
- Update all invocation and checkpoint examples with the new fields, and
  add positive/negative tests for every new check.
- Update `references/CONTRACT.md` to document the new fields and checks.

## Why
- The fifth review-code-change pass on #96 found: (1) the
  checkpoint/terminal-result cross-check omitted repository, branch, and
  publication policy, the same drift class four prior fixes already closed
  for the invocation/checkpoint pair; and (2) two schemas omitted fields
  `design/review-fix-loop.md` explicitly enumerates for the exact sections
  this ticket implements, which would otherwise require a breaking schema
  revision in #97/#98.
- Authorized by the coordinator as one further fix/re-review cycle for
  this ticket, explicitly bundling in the deferred design-field gap so it
  does not need its own future breaking revision.

Refs #96
Supports #95
Correct a misattributed SHA from an earlier entry (it described commit
3d240c6 but cited 86c7df2, a different docs-only commit), backfill the
missing entry for that docs-only commit, backfill the SHA onto the prior
fix entry, and add today's new fix entry, per this repository's changelog
convention.
…commit provenance

## Summary
- Enumerate the complete, closed set of invariant identity fields that must
  agree across invocation, checkpoint, and terminal-result (invocation_id,
  repository, branch, original fix-cycle budget, publication.policy, initial
  head, initial comparison base) in a new `references/CONTRACT.md` table, and
  verify both cross-document functions (`validate_checkpoint_against_invocation`
  and `validate_terminal_against_checkpoint`) check the exact same set.
- Close the one remaining gap the table surfaced: `validate_checkpoint_against_invocation`
  was still missing the `branch` check `validate_terminal_against_checkpoint`
  already had.
- Close the commit-provenance gap in `validate_terminal_result`: `created_commits`
  must equal `head_history[1:]` in order; a `selected` finding disposition
  requires `fix_commit_sha`, a `declined` one must not carry one, and any
  present `fix_commit_sha` must appear in `created_commits`.
- Document, rather than silently omit, the one part of that recommendation
  deliberately not enforced: `unpushed_commits` is not constrained to be a
  subset of `created_commits`/`head_history`, because a resumed or
  already-diverged invocation can legitimately report local commits ahead of
  its recorded source that predate this invocation's own `head.initial` and
  were never created by it — recovery/resume semantics are this ticket's
  stated non-goal, so this boundary is recorded rather than guessed at.
- Add 8 new tests (2 for the branch cross-check, 4 for commit provenance, plus
  the field-set enumeration is exercised transitively by the existing
  positive/negative cross-document tests).

## Why
- The sixth review-code-change pass found the checkpoint↔invocation branch
  gap; the coordinator asked for a systematic sweep (enumerate the complete
  invariant field set, check it symmetrically in both cross-document
  functions, rather than patching one field per cycle) plus the still-open
  commit-provenance recommendation from the prior cycle, in one pass.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
…ntity closure

## Summary
- Add the two remaining design-required checkpoint fields:
  `preserved_failed_attempts` (mirroring terminal-result's shape) and
  `pull_request` ({repository, number}, distinct from
  `publication.pull_request`'s push-mechanics fields).
- `validate_checkpoint` now requires `preserved_failed_attempts` to have
  exactly one entry per `cycle_attempts` entry whose outcome is `failed`
  or `interrupted`.
- Add a shared `_pull_request_identity_mismatch` helper and wire it into
  both `validate_checkpoint_against_invocation` and
  `validate_terminal_against_checkpoint`, so the optional pull-request
  identity is cross-checked wherever two adjacent documents both carry it
  — closing the same drift class the last several cycles have been
  closing for the required invariant fields.
- Update all three checkpoint examples and add 12 new tests (positive and
  negative, for both the preserved-attempts count rule and the
  pull-request cross-checks in both directions).
- Document all of this in `references/CONTRACT.md`.

## Why
- The seventh review-code-change pass found `checkpoint.schema.json`
  still missing two fields `design/review-fix-loop.md` explicitly
  requires as durable-checkpoint content, mirroring a completeness gap
  the fifth pass already closed for two other fields. Rather than patch
  one field per cycle again, this closes the checkpoint schema
  completely against the design's enumerated content and extends the
  cross-document invariant enforcement to cover the newly added optional
  pull-request identity too, so it does not become the next cycle's
  single-field gap.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
…e ahead/behind

## Summary
- Add the three remaining design-required terminal-result fields:
  `worktree`, `resume_status`
  (`not_resumed`/`resumed`/`resume_unavailable`), and
  `unresolved_or_deferred_findings`.
- Require `ahead_by` and `behind_by` alongside the head fields whenever
  `source.status` is `bound`, in both `validate_checkpoint` and
  `validate_terminal_result` — a bound source that reports a head but
  omits the ahead/behind counts does not satisfy the design's "local
  ahead/behind state" requirement.
- Update all four terminal-result examples with the new fields and add 8
  new tests (schema-presence and bound-source rejection, both
  directions).
- Document all of this in `references/CONTRACT.md`.

## Why
- The eighth review-code-change pass found two blocking gaps
  (`terminal-result.schema.json` missing `worktree` and `resume_status`,
  both explicitly enumerated in `design/review-fix-loop.md`'s Terminal
  result contract section) and one strong recommendation (bound sources
  silently omitting ahead/behind counts). While closing those, a
  systematic re-read of the same design section surfaced one more
  explicitly named field this candidate had not yet modeled
  (`unresolved_or_deferred_findings`), addressed in the same pass rather
  than deferring it to a ninth cycle.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
… cleanliness

## Summary
- Add a required `reviewer_identity` (non-empty string) field to
  `review_records` in both `checkpoint.schema.json` and
  `terminal-result.schema.json`, distinct from the invocation-invariant
  `review_independence` enum, so a fresh-subagent reviewer's actual
  per-pass identity can be recorded and distinguished across heads.
- Add a `validate_invocation` check rejecting non-empty
  `candidate.worktree.staged`/`unstaged`/`untracked`, leaving `ignored`
  unconstrained, matching the design's "every invocation requires a
  dedicated globally clean worktree" hard precondition.
- Update all 7 affected examples with distinct per-pass
  `reviewer_identity` values, and add 8 new tests (missing-field
  rejection and a distinguishing-identities test for both schemas; one
  rejection test per dirty worktree category plus a passing test proving
  `ignored` is unconstrained).
- Document both additions in `references/CONTRACT.md`.

## Why
- The ninth review-code-change pass — an intentionally maximal, final
  re-derivation of the design's complete field lists rather than trusting
  prior passes were exhaustive — found both gaps: the design's Terminal
  result contract names "reviewer identities" as required content and its
  Validation strategy requires a test for "different reviewer identities
  per head," which the two-valued `review_independence` enum alone cannot
  satisfy; and `validate_invocation` captured full worktree state but
  never used it to reject a dirty worktree, despite the design stating
  that blocks startup.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
…a mutation attempt

## Summary
- `_check_converged_requires_clean_evidence` now rejects `converged` when
  *any* `review_records` entry has a non-empty `mutation_attempts`, not
  only the final-head-bound entry.
- Add two negative tests: a mutation attempt on the final record, and one
  on a non-final (earlier-head) record, both rejected.
- Document the rule in `references/CONTRACT.md`.

## Why
- The tenth review-code-change pass found that an attempted reviewer
  mutation on an earlier pass did not block `converged`, as long as a
  later pass on a different head came back clean and enforced. Design's
  "Reviewer write prevention" section states an attempted prohibited
  mutation invalidates that pass even if the runtime blocked it,
  regardless of a later clean pass — so this closes the last gap in the
  converged-evidence check.

Refs #96
Supports #95
Backfill SHAs onto the prior review-fix-loop entries and add today's new
fix entry, per this repository's changelog convention.
@shaug
shaug merged commit 2750038 into main Jul 30, 2026
1 check passed
@shaug
shaug deleted the claude/96-review-fix-loop-contracts branch July 30, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant